Add a way to say "all fields" in the projection #155

Dragomir-Ivanov · 2017-10-11T14:07:56Z

This enables us to get implicitly all fields of a resource, when sub-resources are requested #147

GET http://192.168.0.103:3001/api/clients/59a1a17aeaba9ebd0180abb0?fields=*,services{ownOrder},visits{name,appointmentDT,duration}

This makes the following two requests equivalent:

GET http://192.168.0.103:3001/api/clients/59a1a17aeaba9ebd0180abb0?fields=*,services{*},visits{name,appointmentDT,duration}

GET http://192.168.0.103:3001/api/clients/59a1a17aeaba9ebd0180abb0?fields=*,services,visits{name,appointmentDT,duration}

rs · 2017-10-12T01:48:01Z

schema/query/projection_evaluator.go

@@ -42,7 +42,7 @@ func (p Projection) Eval(ctx context.Context, payload map[string]interface{}, rs
 func evalProjection(ctx context.Context, p Projection, payload map[string]interface{}, validator schema.Validator, rbr *referenceBatchResolver) (map[string]interface{}, error) {
 	res := map[string]interface{}{}
 	resMu := sync.Mutex{} // XXX use sync.Map once Go 1.9 is out
-	if len(p) == 0 {
+	if len(p) == 0 || p[0].Name == "*" {


What if * is supplied with other fields? Shouldn't we reject it?

I am not sure. Having address,* will have all fields address included. I can add a condition, where * to be the only predicate, else bail out, but It doesn't feel right.

@smyrman can you comment on this as well.

I am not completely sure I follow your comment @rs.

I thought the whole benefit of the syntax was that you could do fields="*,sub1{},sub2{},hiddenField", to expand sub1 and sub2 and maybe explicitly include hiddenField, while still including all fields that where not explicitly mentioned.

Am I missing something?

Or do you mean support address,* as well as *,address? Or at least given an error if we do address,*, without having implemented support for it?

We should do one of those :-)

When I talk abut sub-resources, I was thinking about sub-resource connected via Resource.Bind. In addition, we need to be aware of what should happen to hidden fields.

resourceA := index.Bind("parent", schema.Schema{ Fields: { "showMe": {}, "hiddenField": {Hidden: true}, }, ...) rscParent := index.Bind("Parent", schema.Schema{ Fields: schema.Fields{ "showMe": {}, "hiddenField": {Hidden: true}, }, }, storeA, conf) // afterc calling rscA.Bind, "sub" will now appear as a `schema.Connection` in `rscA` on a special _field override list_. rscA.Bind("sub", "parent", schema.Schema{ Fields: schema.Fields{ "parent": {Validator: &schema.Reference{Path:"parent"}}, "a": {}, "hiddenField": {Hidden: true}, }, })

My assumption wold be that http GET /parent?fields=* is equivalent to http GET /parent

However, when you ask for either /parent?fields=*,sub{hiddenField} or /parent?fields=sub{hiddenField},* what do you get?

So I think @rs point in this case is, if this is expanded to either fields=showMe,sub,sub{hiddenField} or fields=sub{hiddenField},showMe,sub (notice that sub is listed twice, but with different field selections), which fields will sub really show? Will it show all fields, or just the sub IDs? Clearly the intention of the user would probably be to display only hiddenField from the sub resource.

Btw, The test you showed us was really nice @Dragomir-Ivanov. But I am not sure I would want * to have precedence...

This asks another question too, should fields=foo,foo or fields=f:foo,f:bar raise an error or just result in a "last win"?

@rs Good question, I guess we should just pick one, and keep it until it proves wrong. I vote for last win.

That’s the current implementation. We should add tests and document it then.

rs · 2017-10-12T01:49:30Z

schema/query/projection_parser.go

@@ -178,7 +178,7 @@ func (p *projectionParser) scanFieldName() string {
 	field := []byte{}
 	for p.more() {
 		c := p.peek()
-		if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '-' {
+		if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '-' || c == '*' {


This will allow * in field names, which is not what we want right? The * should only be allowed by itself.

Yes, will fix this.

rs · 2017-10-12T01:51:47Z

schema/query/projection_validator_test.go

@@ -50,6 +50,7 @@ func TestProjectionValidate(t *testing.T) {
 		{`with_params(foo:3.14)`, errors.New("with_params: invalid param `foo' value: not an integer")},
 		{`with_params(bar:1)`, errors.New("with_params: invalid param `bar' value: not a Boolean")},
 		{`with_params(foobar:true)`, errors.New("with_params: invalid param `foobar' value: not a string")},
+    {`*`, nil},


Please add cases for:

Fields with * in its name.

* followed by valid fields.

Valid fields followed by *.

Also, beware indentation. Are you using gofmt?

I will add the cases. Indentation problem was due to resolving merge conflict with GitHub Web GUI.

smyrman · 2017-10-17T23:12:00Z

schema/query/projection_evaluator.go

+func checkStar(p Projection) (bool, error) {
+	hasStar := false
+	s := ""
+	for i := range p {


This for loop is a bit awkward...

Could not this whole test be just:

func checkStar(p Projection) (bool, error) { if len(p) == 1 && p[0].Name == "*" && len p[0].Children == 0 { return true, nil } if len(p) <= 1 { return false, nil } for i := range p { if p[i].Name == "*" && len p[i].Children == 0: { return false, fmt.Errorf("%s: invalid value: * must be the only field", p) } } return false, nil }

This may give a slightly more clear LOS (Line of Sight).

Otherwise I am now OK with * only being allowed as the only parameter, as long as this now expands all references; at least initially. May place nice (end-user-wise) with the suggesrion of using * to expand references in schema.Dict and schema.Array

#138

Thanks for the suggestions @smyrman. However expanding referenced fields, is what I wanted to avoid in the first place. My indention was to provide extension to this requests:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55

Which will return only non referenced fields, so making this request:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=*,visits

Is to actually add the visits referenced resource, without mentioning all the non referenced fields.

Initially I though this as a low hanging fruit, but it gets more complicated. It is not big deal to enumerate all the fields explicitly, so I think I will just abandon this PR.

It is not big deal to enumerate all the fields explicitly, so I think I will just abandon this PR.

Alright 👍 Might be wise for now. This is also the way we use it.

Just for my clarification though.

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=*,visits

Is to actually add the visits referenced resource, without mentioning all the non referenced fields.

This is also what I though was the initial intent. However, my (perhaps wrongful) understanding of the final PR was that it would expand "visits", all other references and maybe (not clear to me) all fields that are hidden by default (Field.Hidden == true) if you did this:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=*

If we rather want the initially described behaviour, I suppose the following might work:

// expandStar replace a single `*` in projection with all fields at the position of `*`. // duplicated fields will be handled according to the last-win principal. expandStart(p *Projection, schema.Fields) { ... }

And then no other changes. This will also ensure errors later if *is part of a field-name to be as before without the extra checks added in this PR, and withourt accepting * in the legal field character list.

Anyway, still maybe best to leave it for now, and consider it a bit closer.

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=*

The request above would have been equivalent to this:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55

No referenced fields, no hidden fields.
Actually there is no way of getting the hidden fields with the current code. Mentioning them explicitly in the fields= parameter results in the following error:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=nameTokens "Invalid `fields` parameter: nameTokens: hidden field"

Client = schema.Schema{ Fields: schema.Fields{ "id": mongo.ObjectIDField, ... "nameTokens": { Hidden: true, },

Closing the PR now.

Thanks for your effort, even if it was not merged this time.

Dragomir-Ivanov mentioned this pull request Oct 11, 2017

Returning whole resource, along with its sub-resouce #147

Closed

rs requested changes Oct 12, 2017

View reviewed changes

rs requested a review from smyrman October 12, 2017 01:54

Add a way to say "all fields" in the projection

f9f124f

smyrman reviewed Oct 17, 2017

View reviewed changes

Dragomir-Ivanov closed this Oct 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to say "all fields" in the projection #155

Add a way to say "all fields" in the projection #155

Dragomir-Ivanov commented Oct 11, 2017 •

edited

Loading

rs Oct 12, 2017

Dragomir-Ivanov Oct 12, 2017 •

edited

Loading

Dragomir-Ivanov Oct 12, 2017

smyrman Oct 12, 2017

smyrman Oct 12, 2017 •

edited

Loading

smyrman Oct 13, 2017 •

edited

Loading

smyrman Oct 13, 2017

rs Oct 14, 2017

Dragomir-Ivanov Oct 14, 2017

rs Oct 14, 2017

rs Oct 12, 2017

Dragomir-Ivanov Oct 12, 2017

rs Oct 12, 2017

Dragomir-Ivanov Oct 12, 2017

smyrman Oct 17, 2017 •

edited

Loading

smyrman Oct 18, 2017

Dragomir-Ivanov Oct 18, 2017

smyrman Oct 18, 2017 •

edited

Loading

Dragomir-Ivanov Oct 18, 2017

Dragomir-Ivanov Oct 18, 2017

smyrman Oct 18, 2017

Add a way to say "all fields" in the projection #155

Add a way to say "all fields" in the projection #155

Conversation

Dragomir-Ivanov commented Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Dragomir-Ivanov Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

smyrman Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dragomir-Ivanov commented Oct 11, 2017 •

edited

Loading

Dragomir-Ivanov Oct 12, 2017 •

edited

Loading

smyrman Oct 12, 2017 •

edited

Loading

smyrman Oct 13, 2017 •

edited

Loading

smyrman Oct 17, 2017 •

edited

Loading

smyrman Oct 18, 2017 •

edited

Loading